Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement ByteStr and ByteString types #135073

Merged
merged 8 commits into from
Jan 24, 2025
Merged

Implement ByteStr and ByteString types #135073

merged 8 commits into from
Jan 24, 2025
+1,412 −14

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jan 3, 2025 •
edited
Loading

Approved ACP: rust-lang/libs-team#502
Tracking issue: #134915

These types represent human-readable strings that are conventionally,
but not always, UTF-8. The Debug impl prints non-UTF-8 bytes using
escape sequences, and the Display impl uses the Unicode replacement
character.

This is a minimal implementation of these types and associated trait
impls. It does not add any helper methods to other types such as [u8]
or Vec<u8>.

I've omitted a few implementations of AsRef, AsMut, Borrow,
From, and PartialOrd, when those would be the second implementation
for a type (counting the T impl) or otherwise may cause inference
failures. These impls are important, but we can attempt to add them
later in standalone commits, and run them through crater.

In addition to the bstr feature, I've added a bstr_internals feature
for APIs provided by core for use by alloc but not currently
intended for stabilization.

This API and its implementation are based heavily on the bstr crate
by Andrew Gallant (@BurntSushi).

r? @BurntSushi

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 3, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett

This comment was marked as resolved.

Sign in to view
@joshtriplett joshtriplett mentioned this pull request Jan 3, 2025
Open
3 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett

This comment was marked as resolved.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum

This comment was marked as resolved.

Sign in to view
@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. needs-crater This change needs a crater run to check for possible breakage in the ecosystem. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2025
@joshtriplett joshtriplett force-pushed the bstr branch 2 times, most recently from b539a8c to f035281 Compare January 5, 2025 16:51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
@bors
Auto merge of rust-lang#135073 - joshtriplett:bstr, r=<try>
94d9044 
Implement `ByteStr` and `ByteString` types

Approved ACP: rust-lang/libs-team#502
Tracking issue: rust-lang#134915

These types represent human-readable strings that are conventionally,
but not always, UTF-8. The `Debug` impl prints non-UTF-8 bytes using
escape sequences, and the `Display` impl uses the Unicode replacement
character.

This is a minimal implementation of these types and associated trait
impls. It does not add any helper methods to other types such as `[u8]`
or `Vec<u8>`.

I've omitted a few implementations of `AsRef`, `AsMut`, and `Borrow`,
when those would be the second implementation for a type (counting the
`T` impl), to avoid potential inference failures. We can attempt to add
more impls later in standalone commits, and run them through crater.

In addition to the `bstr` feature, I've added a `bstr_internals` feature
for APIs provided by `core` for use by `alloc` but not currently
intended for stabilization.

This API and its implementation are based *heavily* on the `bstr` crate
by Andrew Gallant (`@BurntSushi).`

r? `@BurntSushi`
@bors
Copy link
Contributor

bors commented Jan 12, 2025

⌛ Trying commit 22a4ec3 with merge 94d9044...

@bors
Copy link
Contributor

bors commented Jan 12, 2025

☀️ Try build successful - checks-actions
Build commit: 94d9044 (94d9044c53e7ebf518ede2bec35f06c51ca83f62)

@joshtriplett
Copy link
Member Author

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-135073-1/retry-regressed-list.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-135073-2 created and queued.
🤖 Automatically detected try build 94d9044
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-135073-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-135073-2 is completed!
📊 0 regressed and 0 fixed (167 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 13, 2025
@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 13, 2025 •
edited
Loading

Well, that's definitive.

This is ready for review.

@joshtriplett joshtriplett removed the needs-crater This change needs a crater run to check for possible breakage in the ecosystem. label Jan 13, 2025
BurntSushi
BurntSushi approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

Was there discussion on ByteStr verus BStr? I guess the latter is a bit more opaque, so I think ByteStr is probably the right choice for std. I do like the succinctness of BStr though. (And similarly for ByteString versus BString.)

@Sky9x
Copy link
Contributor

Sky9x commented Jan 22, 2025

Could an impl CloneToUninit for ByteStr be added? cc #126799

@joshtriplett
Copy link
Member Author

This is awesome!

Thank you!

Was there discussion on ByteStr verus BStr? I guess the latter is a bit more opaque, so I think ByteStr is probably the right choice for std. I do like the succinctness of BStr though. (And similarly for ByteString versus BString.)

I like the succinctness of BStr and BString as well, but others in the libs-api meeting felt those were too opaque. The deciding factor that led me to not push further for the shorter names was that the longer names don't conflict with the bstr crate, making it safer to (for instance) place them in the prelude in the future, and making it easier for crates that need to provide impls for both.

@joshtriplett
Copy link
Member Author

Could an impl CloneToUninit for ByteStr be added? cc #126799

Sure, I can do that.

@Skgland
Copy link
Contributor

Skgland commented Jan 22, 2025

I noticed that ByteString isn't allocator aware. Is this intentional? Going from non to one (defaulted) generic argument is making the addition of allocator awareness difficult for String in #101551 (though that would probably only be a problem if ByteString didn't become allocator aware prior to stabilization which is still far out with it not even being merged to nightly).

@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 22, 2025 •
edited
Loading

@Skgland No objection to adding that, but that would be a complex change on top of this, so I would prefer to defer it to another PR.

@BurntSushi
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2025

📌 Commit 865471f has been approved by BurntSushi

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 23, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2025
@bors
Auto merge of rust-lang#135947 - matthiaskrgr:rollup-k9jpfls, r=matth…
1c9837d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#135073 (Implement `ByteStr` and `ByteString` types)
 - rust-lang#135492 (Add missing check for async body when suggesting await on futures.)
 - rust-lang#135766 (handle global trait bounds defining assoc types)
 - rust-lang#135880 (Get rid of RunCompiler)
 - rust-lang#135908 (rustc_codegen_llvm: remove outdated asm-to-obj codegen note)
 - rust-lang#135911 (Allow `arena_cache` queries to return `Option<&'tcx T>`)
 - rust-lang#135920 (simplify parse_format::Parser::ws by using next_if)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 08d5b23 into rust-lang:master Jan 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 24, 2025
@joshtriplett joshtriplett deleted the bstr branch January 24, 2025 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@BurntSushi BurntSushi BurntSushi approved these changes

Assignees

@BurntSushi BurntSushi

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
10 participants
@joshtriplett @rust-log-analyzer @Mark-Simulacrum @bors @craterbot @Sky9x @Skgland @BurntSushi @Noratrieb @rustbot